Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Feature/skill api #1822

Merged
merged 4 commits into from
Feb 23, 2021
Merged

Feature/skill api #1822

merged 4 commits into from
Feb 23, 2021

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Sep 28, 2018

Description

This is a suggestion for an easy way for skills to publish and use other skills shared methods. The idea is that for example the alarm skill wants to

The skill api object is generated from the skill's public_api property. It's a dict where each key is turned into a method on the api object. The method is defined as

  "api_method": {
    "type": message type string
    "func": handler method
    "help": help string for cli
  }

This dict is generated from methods in the skill tagged with the skill_api_method decorator.

Example skill:

from mycroft.skills import MycroftSkill, skill_api_method

class Test2(MycroftSkill):
    @skill_api_method
    def speak_meth(self, message):
        """Speak the test sentence."""
        self.speak('This is a test')
 
    @skill_api_method
    def speak_meth2(self, message):
        """Speak another test sentence."""
        self.speak('This is another test')

To use this skill from another skill

import the SkillApi class

from mycroft.skills.api import SkillApi

and then get an api object and use that to trigger the skill:

my_skill = SkillApi.get('myskill.forslund')
my_skill.speak_meth()

This also adds a help interface to the api in the CLI, :api SKILL will list available api methods.

Contributor license agreement signed?

CLA [ Yes ]

@pep8speaks
Copy link

pep8speaks commented Sep 28, 2018

Hello @forslund! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-08 07:02:54 UTC

@penrods penrods added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Oct 1, 2018
@JarbasAI JarbasAI mentioned this pull request Oct 2, 2018
@JarbasAl
Copy link
Contributor

JarbasAl commented Oct 9, 2018

what about some methods to handle binary data? base64 encode/decode basically

penrods
penrods previously approved these changes Jun 5, 2019
Copy link
Contributor

@penrods penrods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks solid to me. I'm good with merging it.

@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Jul 12, 2019
@forslund
Copy link
Collaborator Author

forslund commented Dec 8, 2019

Rebase Needed

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund force-pushed the feature/skill-api branch 2 times, most recently from 38de5f6 to b3a4bf9 Compare July 6, 2020 08:02
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

return wrapper

methods = [a for a in get_non_properties(self)
if hasattr(a, '__name__')]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of my decorated methods seem to have a __name__ attribute. However they do get assigned the api_method.

Example Skill with an API method: https://github.com/krisgesling/skill-api-source-skill

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get True If I do self.log.info(hasattr(self.speak_from_other_skill, '__name__')) on your skill

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check does look a bit wonky though. Let's see if I can improve it...

Copy link
Contributor

@krisgesling krisgesling Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, same if I log from the Skill itself, but logging from here returns False:

        if self.skill_id == 'skill-api-source-skill':
            for a in get_non_properties(self):
                LOG.info("{} has __name__: {}".format(a, hasattr(a, '__name__')))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, the get_non_properties() returns a string not a reference to the attribute, so looks like we need this to be:

        methods = [a for a in get_non_properties(self)
                   if hasattr(getattr(self, a), '__name__')]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great catch...how has this ever worked?

Copy link
Collaborator Author

@forslund forslund Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed your suggested fix and updated the CLI code to handle :api without a skill argument. Gonna rebase this and fix the conflict, then I'll see if I can re-verify the functionality

@forslund
Copy link
Collaborator Author

Rebased and fixed the conflict

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Sep 23, 2020
@krisgesling krisgesling added Status: To be reviewed Concept accepted and PR has sufficient information for full review Type: Enhancement - roadmapped Implementation of a feature that is a priority on the roadmap. and removed tagged for review and potential merge labels Sep 24, 2020
@krisgesling krisgesling linked an issue Sep 24, 2020 that may be closed by this pull request
@krisgesling
Copy link
Contributor

Hey, this is working well now.

Have you thought much about what some unit tests for this might look like?

Also, I just did tiny edit of the top comment to fix a typo in the example code.

@forslund
Copy link
Collaborator Author

forslund commented Feb 3, 2021

Haven't thought too much about it but I think I can write up a small suite of tests for it.

The skill api allows skills to define a public api which can easily be
accessed by other skills over the message bus just as easy as working
with a normal object.

The skill api object is generated from the skill's public_api property. It's a dict where each key is turned into a method on the api object. The method is defined as
  "api_method": {
    "type": message type string
    "func": handler method
    "help": help string for cli
  }

Example skill:

class Test2(MycroftSkill):
    def __init__(self):
        MycroftSkill.__init__(self)
        self.public_api = {
            'speak': {
                'type': 't2.speak',
                'func': self.handle_speak,
                'help': 'speak the test sentence\nand another line\n\nlast'
            },
            'speak2': {
                'type': 't2.speak2',
                'func': self.handle_speak2,
                'help': 'speak the other sentence'
            }
        }

    def handle_speak(self, message):
        self.speak('This is a test')
        self.bus.emit(message.response(data=None))

    def handle_speak2(self, message):
        self.speak('This is another test')
        self.bus.emit(message.response(data=None))
The api methods are now much easier to use, almost transparent. The
current caveat is that only "standarad" python types are acceptable
(int, float, str, list, bool, None) due to the json serialization.

- api methods are now created with the skill_api_method decorator
- both arguments and keyword arguments are sent to the api method
instead of the message object
- api methods now uses a normal return statement instead of having to
handle creating response messages on the bus.

For example if the datetime skill wants to make the datetime string
fetchable simply add the skill_api_method decorator to the
get_display_date method.

    @skill_api_method
    def get_display_date(self, day=None, location=None):
        """Returns the date and time as a string."""
        [...]

The methods return value will be sent back to the caller and can be used
from a skill through

        datetime = SkillApi.get('mycroft-date-time.mycroftai')
        self.log.info(datetime.get_display_date())
@forslund
Copy link
Collaborator Author

forslund commented Feb 6, 2021

Rebased and fixed conflict and added unittests for the key components.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

- Tests for the MycroftSkill side implementation
- Tests for the generated SkillApi objects
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling added Status: Accepted PR has been reviewed and accepted. There must be some reason why it isn't being merged. and removed Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Feb 16, 2021
@Touhey
Copy link

Touhey commented Feb 22, 2021

hello will the skills api be added to pycroft soon?

@krisgesling krisgesling merged commit 38839a1 into MycroftAI:dev Feb 23, 2021
@krisgesling
Copy link
Contributor

Hey @Touhey - if you switch to the dev channel and pull the latest changes, this will now be available :D

We'll also be releasing the next major version of mycroft-core in a few weeks. Then it will be available on the stable / master branch

@Touhey
Copy link

Touhey commented Feb 23, 2021

Hey @Touhey - if you switch to the dev channel and pull the latest changes, this will now be available :D

We'll also be releasing the next major version of mycroft-core in a few weeks. Then it will be available on the stable / master branch

hi thanks for speedy reply where can i find the dev channel for picroft?

@krisgesling
Copy link
Contributor

krisgesling commented Feb 25, 2021

@Touhey - this is the dev channel of mycroft-core so if you SSH into the device, or hook up a keyboard and monitor - the following should work:

cd ~/mycroft-core
mycroft-stop
git checkout dev
git pull
./dev_setup.sh
mycroft-start all

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: Accepted PR has been reviewed and accepted. There must be some reason why it isn't being merged. Type: Enhancement - roadmapped Implementation of a feature that is a priority on the roadmap.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible for skills to share information?
7 participants